Auto-find available port and version-check running instances#13
Conversation
- Bind TCP listener before SMB setup; auto-increment port on AddrInUse (up to 100 tries) so the proxy never fails on a busy port. - Add version to Server header (spiceio/X.Y.Z) so the setup action can detect stale instances. - Setup action now compares running version against installed binary and replaces outdated instances. Bare "Server: spiceio" (pre-versioned builds) is treated as outdated. - Readiness loop discovers actual endpoint from SPICEIO_LOG_FILE, handling port auto-increment correctly.
There was a problem hiding this comment.
Pull request overview
This PR improves local/CI ergonomics for running spiceio by making the server more resilient to port conflicts and enabling the GitHub setup action to detect/replace stale running instances based on a versioned Server header.
Changes:
- Bind the TCP listener before SMB setup and auto-increment the port (up to a limit) when the requested port is already in use.
- Add a versioned
Server: spiceio/X.Y.Zresponse header to support stale-instance detection. - Update the setup composite action to (1) compare running vs installed versions, (2) replace mismatched instances, and (3) discover the actual bound endpoint from the log when the port shifts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/s3/router.rs |
Adds a versioned Server header (spiceio/<pkg version>) to responses. |
src/main.rs |
Binds the TCP listener early and auto-increments the port on AddrInUse. |
.github/actions/setup/action.yml |
Adds version checks/replacement logic and discovers the runtime endpoint from logs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Handle checked_add overflow gracefully instead of panicking when starting port is near u16::MAX. - Use mapfile to collect multiple lsof PIDs (IPv4/IPv6) and kill them all when replacing an old instance. - Truncate SPICEIO_LOG_FILE before starting and use tail -1 to pick the most recent "listening on" line, preventing stale endpoint discovery.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| exit 0 | ||
| # Check if spiceio is already listening on the requested address | ||
| WANT_VERSION=$(spiceio --version | awk '{print $2}') | ||
| RUNNING_HEADER=$(curl -sf -I "http://${SPICEIO_BIND}/" 2>/dev/null | grep -i '^server:.*spiceio' | tr -d '\r' || true) |
There was a problem hiding this comment.
curl -sf -I "http://${SPICEIO_BIND}/" uses HEAD on /. The router returns 405 for service-level requests that are not GET (see src/s3/router.rs around the req_bucket.is_empty() match), and -f suppresses headers on 4xx, so RUNNING_HEADER will usually be empty and the version/skip logic won’t trigger. Use a GET request when checking headers (e.g., request / and capture response headers) or remove -f/target a path that supports HEAD (like /$SPICEIO_BUCKET).
| RUNNING_HEADER=$(curl -sf -I "http://${SPICEIO_BIND}/" 2>/dev/null | grep -i '^server:.*spiceio' | tr -d '\r' || true) | |
| RUNNING_HEADER=$(curl -sS -D - -o /dev/null "http://${SPICEIO_BIND}/" 2>/dev/null | grep -i '^server:.*spiceio' | tr -d '\r' || true) |
| exit 1 | ||
| fi | ||
| ENDPOINT=$(grep 'listening on' "$SPICEIO_LOG" 2>/dev/null | grep -o 'http://[^ ]*' | tail -1 || true) | ||
| if [[ -n "$ENDPOINT" ]] && curl -sf -I "$ENDPOINT/" 2>/dev/null | grep -qi "server: spiceio"; then |
There was a problem hiding this comment.
The readiness probe uses curl -sf -I "$ENDPOINT/" and expects a Server: header. Since / does not support HEAD (router returns 405) and -f hides headers on 4xx, this loop can time out even when the server is up. Consider switching back to a GET-based readiness check and, if you still need headers, capture them from the GET response.
| if [[ -n "$ENDPOINT" ]] && curl -sf -I "$ENDPOINT/" 2>/dev/null | grep -qi "server: spiceio"; then | |
| if [[ -n "$ENDPOINT" ]] && curl -sS -D - -o /dev/null "$ENDPOINT/" 2>/dev/null | grep -qi '^server: spiceio'; then |
| # Old or mismatched version — kill it so we can replace it | ||
| mapfile -t OLD_PIDS < <(lsof -ti "tcp:${SPICEIO_BIND##*:}" -sTCP:LISTEN 2>/dev/null || true) | ||
| if (( ${#OLD_PIDS[@]} > 0 )); then | ||
| echo "replacing spiceio ${RUNNING_VERSION:-unknown} (PID(s) ${OLD_PIDS[*]}) with $WANT_VERSION" | ||
| kill "${OLD_PIDS[@]}" 2>/dev/null || true | ||
| for i in $(seq 1 10); do | ||
| lsof -ti "tcp:${SPICEIO_BIND##*:}" -sTCP:LISTEN >/dev/null 2>&1 || break | ||
| sleep 1 |
There was a problem hiding this comment.
Port extraction via ${SPICEIO_BIND##*:} breaks for IPv6 binds (e.g. [::1]:8333), and the lsof -ti "tcp:..." form is also inconsistent with the repo’s other scripts/workflows (e.g. .github/workflows/ci.yml:80 uses lsof -i ":$PORT" -sTCP:LISTEN -t). Consider parsing the port in an IPv6-safe way and using the same lsof -i ":$PORT" ... pattern here for portability.
Summary
Server: spiceio/X.Y.Zreplaces bareServer: spiceio, enabling the setup action to detect stale instances.Server: spiceio(no version) is treated as outdated.SPICEIO_LOG_FILEso the action'sendpointoutput is correct even when the port shifted.Test plan
cargo check/cargo buildpassesServerresponse header isspiceio/0.4.0